test: raise line coverage to 85%+ across 21 of 22 files below threshold - #604

Review Findings

Reviewed via code-reviewer, security-reviewer, test-writer, and refactor-helper agents against the full diff.

Critical (0)

None.

Warnings (6)

[code-reviewer/refactor-helper] src/pager/error.rs — display_all_variants
Tautological assertion: assert!(...contains("page number") || !...is_empty()). The || !is_empty() disjunct is true for almost any non-empty Display output, so this doesn't actually verify the message. Fix: drop the fallback, assert the exact expected substring like every other variant in the same test does.

[code-reviewer/refactor-helper/test-writer] src/codegen/subquery/pushdown.rs::allows_unqualified_column_without_join
Same shape: assert!(!out.contains("WHERE a = 1") || out.contains("(SELECT a FROM t WHERE a = 1)")). Would pass even if the predicate were silently dropped instead of pushed down. Fix: assert the pushed form directly, matching the sibling test pushes_into_second_join_table one line below.

[refactor-helper] src/vfs/page_source.rs
assert_eq!(page_err.to_string(), format!("{}", page_err)) is a tautology (to_string() is format!("{}")) — asserts nothing about the actual message. Delete or replace with a check against the real expected string.

[test-writer] src/codegen/subquery/flatten.rs — missing adjacent veto cases
subquery_flatten_safe bails on distinct/having/group_by/limit/compound/aggregate, but only distinct/limit/aggregate/join cases are tested. The sibling file pushdown.rs tests all five conditions for the analogous subquery_pushdown_safe. Missing: does_not_flatten_subquery_with_having, does_not_flatten_compound_subquery.

[test-writer] src/codegen/subquery/scalar.rs::scalar_subquery_with_aggregate
assert!(!program.instructions.is_empty()) only re-checks that compile didn't error (already covered by the preceding .unwrap()). Every other test in this file asserts specific opcodes — this one should too.

[test-writer] src/vdbe/exec.rs::vm_debug_and_default_helpers_are_reachable
assert!(debug_str.contains("Vm")) can't meaningfully fail for a #[derive(Debug)] struct named Vm. Coverage-padding disguised as an assertion — either assert something real about the rendered state or drop the assertion and be honest it's a doesn't-panic smoke test.

Suggestions (8)

• [refactor-helper/test-writer] src/codegen/select/join_access.rs and join_order.rs duplicate identical span()/col()/eq()/schema()/binding() test-AST helpers (~80 lines) — worth a shared #[cfg(test)] helper module in codegen/select.rs.
• [refactor-helper] src/codegen/subquery/{flatten,from_clause,pushdown,scalar}.rs each redefine an identical parse(sql) -> Select panic-helper — hoist one copy into codegen/subquery.rs.
• [refactor-helper] src/parser/error.rs's assert_accepted/unsupported/invalid helpers take a redundant src: &str that always duplicates the first argument's literal (~25 call sites) — drop the second parameter.
• [refactor-helper] src/parser/printer.rs's three ~100-line round-trip test functions (expr_display_covers_all_kinds, literal_display_covers_all_kinds, and the insert/delete/create_index/create_view/transaction block) repeat the same match-and-assert shape ~20+ times — collapse to loops / a small generic helper.
• [test-writer] flatten.rs's format!("{:?}", select.where_clause.unwrap()) + .contains("\"s\"") checks (in requalifies_when_sibling_join_present and two others) are fragile — any quoted "s" anywhere in the nested Debug tree satisfies them. pushdown.rs's tests in the same PR use select.to_string() for a tighter check; suggest the same here.
• [test-writer/refactor-helper] src/bin/sqlite-rs/readline.rs::redraw_writes_highlighted_and_plain_prompt_lines and term.rs::write_flush_writes_to_stdout/color_constants_are_expected_escape_codes have no real assertions (pure doesn't-panic or change-detector). Low value but harmless — rename to make the "smoke test" intent explicit, or drop.
• [security-reviewer] src/vfs/unix.rs's new tmp_path()/fastrand_stub() test helper builds temp filenames via PID+timestamp with no atomic create — theoretical TOCTOU, but confined to the test binary, no production/attacker exposure. Optional: use the tempfile crate pattern if this spreads.
• [test-writer] src/bin/sqlite-rs/readline/term.rs staying at 66% coverage is confirmed to be a genuine tty-syscall limitation, not a shortcut — no injectable seam exists between RawMode and the raw termios calls. Worth a follow-up ticket (not this PR) to introduce an injectable Tty trait so RawMode::enable's success path and Drop impl can be tested with a fake.

Unaddressed Requirements

None — all 22 files from issue #603 are addressed; the one remaining gap (term.rs) is explicitly called out in the PR description per the issue's own carve-out clause, and confirmed genuinely hard to close without a design change (see suggestion above).

Strengths

✓ Pure test-only diff — no production code changes, confirmed by all four agents
✓ No new unsafe/dyn Trait outside existing, already-excluded boundaries
✓ No secrets, injection surfaces, or unsafe-in-test-code issues
✓ Vast majority of new tests assert concrete behavior (exact Display strings, opcode sequences, hand-built page bytes) rather than padding for coverage
✓ The one file left below threshold is honestly flagged with a real technical reason, not silently worked around

Summary: 0 critical, 6 warnings, 8 suggestions, 0 unaddressed requirements

🤖 Analysis by Claude

Review Findings Addressed

Fixed all 6 warnings from the review (58b2fc1):

• pager/error.rs::display_all_variants — replaced the A || B tautology with exact expected-string assertions for Page/Vfs/Freelist variants.
• codegen/subquery/pushdown.rs::allows_unqualified_column_without_join — asserts the pushed form directly now.
• vfs/page_source.rs — replaced the to_string() == format!("{}", ...) self-comparison with the real expected message.
• codegen/subquery/flatten.rs — added does_not_flatten_subquery_with_having and does_not_flatten_compound_subquery, closing the gap vs. the sibling pushdown.rs coverage.
• codegen/subquery/scalar.rs::scalar_subquery_with_aggregate — now asserts AggStep/AggFinal opcodes instead of just "non-empty instructions".
• vdbe/exec.rs::vm_debug_and_default_helpers_are_reachable — now asserts real register state renders in the Debug output.

The 8 suggestions (test-helper duplication across sibling files, fragile Debug-format assertions in flatten.rs, a couple of no-assertion smoke tests, test temp-file naming, and the injectable-Tty-trait idea for term.rs) are left as-is per the fix plan — style/duplication cleanup better suited to a dedicated follow-up than bundled into this coverage PR, and the Tty trait is a design decision for its own ticket.

cargo test --lib (922 passed), clippy, and fmt all clean.

🤖 Analysis by Claude